You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Replaces custom span-based draggable sliders with native <input type="range" class="frm-slider"> across templates, web components, settings JS, and CSS; shifts value handling to input/change events, unit-aware max/value logic, and CSS --frm-fill for visual fill.
Replaced span-based slider markup with <input type="range" class="frm-slider"> for all slider variants. Independent-field range value is (int) $field['value'] when $component['unit_measurement'] is non-empty; otherwise uses escaped string value.
Web Component DOM js/src/web-components/frm-range-slider-component/frm-range-slider-component.js
createSliderTrack(value) → createSliderTrack(value, maxValue) now returns an <input type="range"> with min="0", max from maxValue, and value from the passed value; call site updated accordingly.
Removed dragging/width/position APIs and pixel-to-value helpers. Added initListeners() and initFill(). Wiring now uses input for live fill/text sync and change for commit, computes fill from rangeInput.value vs. unit-aware max, updates fullValue, and updated updateOnUnitChange(rangeInput, valueInput, index).
Bootstrapping adjusted to instantiate the refactored slider logic and rely on range/text event-driven propagation; removed prior dragging/position initialization paths.
Removed .frm-slider-active-track and bullet/label rules. Added input[type="range"].frm-slider styling driven by --frm-fill linear-gradient, cross-browser thumb rules (::-webkit-slider-thumb, ::-moz-range-thumb, ::-moz-range-progress), :focus-visible outlines, min-height: 0 for value inputs/selects, and cursor: pointer for slider icons.
Sequence Diagram
sequenceDiagram
participant User
participant RangeInput
participant SliderJS as Slider Component (JS)
participant DepUpdater as Dependent Updater / Hidden Input
User->>RangeInput: drag / change value
RangeInput->>SliderJS: input event (live)
SliderJS->>RangeInput: update --frm-fill (visual)
SliderJS->>DepUpdater: update hidden value / fullValue
SliderJS->>DepUpdater: dispatch change event (on commit)
Loading
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
"I nibble code with nimble paws,
Swapped tracks for ranges without a pause.
CSS fills shimmer, inputs sing,
Events now guide the slider's spring.
Hooray — a simpler hopping cause! 🐇"
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title 'Improve accessibility of slider setting' directly aligns with the PR's stated objective to update slider settings for improved accessibility and addresses reported accessibility issues.
Docstring Coverage
✅ Passed
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch improve-slider-setting-accessibility
Tip
💬 Introducing Slack Agent: The best way for teams to turn conversations into code.
Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly
Built for teams:
Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls
One agent for your entire SDLC. Right inside Slack.
We reviewed changes in b00df64...3e9d3f1 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
The reason will be displayed to describe this comment to others. Learn more.
Variable $component might not be defined
A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.
Clamp the current value when the selected unit lowers the max.
After switching units, Line 206 updates the range max but the current value is reused unchanged. A value like 250px becomes 250% in hidden state, while the slider can only represent up to 100.
Representative fix
element.classList.remove( 'frm-disabled', 'frm-empty' );
- rangeInput.max = this.getMaxValue( unit, index );- this.options[ index ].fullValue = valueInput.value + unit;+ const maxValue = this.getMaxValue( unit, index );+ const nextValue = Math.min(+ Math.max( Number.parseInt( valueInput.value, 10 ) || 0, 0 ),+ maxValue+ );+ rangeInput.max = maxValue;+ rangeInput.value = nextValue.toString();+ valueInput.value = nextValue.toString();+ this.options[ index ].fullValue = `${ nextValue }${ unit }`;
this.updateValue( element, this.options[ index ].fullValue );
this.updateFill( rangeInput, index );
this.triggerValueChange( index );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/src/settings-components/components/slider-component.js` around lines 188 -
210, In updateOnUnitChange, when changing units the code updates rangeInput.max
via this.getMaxValue(unit, index) but does not clamp the current numeric value —
causing out-of-range values (e.g., 250px → 250%) to persist; fix by parsing the
numeric part from valueInput.value, compute const max = this.getMaxValue(unit,
index), clamp the numeric value to Math.min(numericValue, max), set
rangeInput.value and valueInput.value to the clamped numeric (and append the
unit where needed), update this.options[index].fullValue accordingly, then call
this.updateValue(element, ...) followed by this.updateFill(rangeInput, index)
and this.triggerValueChange(index).
Seed the hidden input with the serialized slider value, not options.
Line 445 passes the whole options object into createSliderHiddenInputValue(). That method assigns its argument to input.value, so single-value web-component sliders start out with an invalid hidden value.
Verify each finding against the current code and only fix it if needed.
In
`@js/src/web-components/frm-range-slider-component/frm-range-slider-component.js`
around lines 444 - 446, The code currently passes the whole options object into
createSliderHiddenInputValue (via
valueContainer.append(this.createSliderHiddenInputValue(options))), which makes
the hidden input get an invalid object value; instead pass the slider's
serialized/current value (not options) — e.g., derive the current value from
options.value or the component getter (like this.getValue() or this.value),
serialize it to a string, and call
createSliderHiddenInputValue(serializedValue); also ensure
createSliderHiddenInputValue expects/assigns a string to input.value rather than
an object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/views/styles/components/templates/slider.php`:
- Line 11: The range inputs (class "frm-slider") lack accessible names — add an
aria-label (or aria-labelledby) to each range input using the same pattern/value
used by the sibling text input/select in this template so screen readers
announce the slider; update the input element with the aria attribute for every
occurrence of the range (the ones currently rendered with class "frm-slider" at
the listed positions) to use the same escaped label value used by the sibling
control.
In `@js/src/settings-components/components/slider-component.js`:
- Around line 81-93: The change handler currently only enforces the upper bound,
allowing negative or non-numeric values to reach updateValue; parse and
normalize the manual input first by reading event.target.value, converting with
parseInt(..., 10), checking isNaN and clamping it between the minimum (use
getMinValue if available or 0) and this.getMaxValue(unit, index), then write the
normalized/clamped value back to valueInput.value and rangeInput.value before
calling this.updateFill(rangeInput, index), this.options[index].fullValue =
this.updateValue(element, valueInput.value + unit), and
this.triggerValueChange(index) so slider and hidden value stay in sync.
In
`@js/src/web-components/frm-range-slider-component/frm-range-slider-component.js`:
- Around line 319-330: The range input created in createSliderTrack currently
doesn't receive the passed ariaLabel, so assistive tech sees it as unnamed;
update createSliderTrack(value, maxValue) to accept or read the ariaLabel and
set it on the native element (e.g., slider.setAttribute('aria-label', ariaLabel)
or slider['ariaLabel']=ariaLabel) and do the same propagation in the other
range-creation function referenced around lines 437-442 (the alternate range
control factory) so both native input elements receive the aria-label from the
component prop.
---
Outside diff comments:
In `@js/src/settings-components/components/slider-component.js`:
- Around line 188-210: In updateOnUnitChange, when changing units the code
updates rangeInput.max via this.getMaxValue(unit, index) but does not clamp the
current numeric value — causing out-of-range values (e.g., 250px → 250%) to
persist; fix by parsing the numeric part from valueInput.value, compute const
max = this.getMaxValue(unit, index), clamp the numeric value to
Math.min(numericValue, max), set rangeInput.value and valueInput.value to the
clamped numeric (and append the unit where needed), update
this.options[index].fullValue accordingly, then call this.updateValue(element,
...) followed by this.updateFill(rangeInput, index) and
this.triggerValueChange(index).
In
`@js/src/web-components/frm-range-slider-component/frm-range-slider-component.js`:
- Around line 444-446: The code currently passes the whole options object into
createSliderHiddenInputValue (via
valueContainer.append(this.createSliderHiddenInputValue(options))), which makes
the hidden input get an invalid object value; instead pass the slider's
serialized/current value (not options) — e.g., derive the current value from
options.value or the component getter (like this.getValue() or this.value),
serialize it to a string, and call
createSliderHiddenInputValue(serializedValue); also ensure
createSliderHiddenInputValue expects/assigns a string to input.value rather than
an object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b8e7f31-c93b-42b6-b989-ebdd7ea6bd08
📥 Commits
Reviewing files that changed from the base of the PR and between b00df64 and b3a5246.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Add an accessible name to each range input.
These sliders are now native controls, but they still render without a label. Screen readers will announce an unnamed slider here because only the sibling text input/select has aria-label.
Apply the same pattern to the other range inputs in this template.
Also applies to: 27-27, 43-43, 59-59, 75-75, 91-91, 114-114, 133-133, 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/views/styles/components/templates/slider.php` at line 11, The range
inputs (class "frm-slider") lack accessible names — add an aria-label (or
aria-labelledby) to each range input using the same pattern/value used by the
sibling text input/select in this template so screen readers announce the
slider; update the input element with the aria attribute for every occurrence of
the range (the ones currently rendered with class "frm-slider" at the listed
positions) to use the same escaped label value used by the sibling control.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Normalize manual text input before persisting it.
This path only checks the upper bound. Negative or non-numeric input can still reach updateValue(), while the native range clamps or rejects it, so the hidden value and visible slider diverge.
Representative fix
valueInput.addEventListener( 'change', event => {
const unit = element.querySelector( 'select' ).value;
+ const nextValue = Number.parseInt( event.target.value, 10 );++ if ( Number.isNaN( nextValue ) ) {+ return;+ }++ const clampedValue = Math.min(+ Math.max( nextValue, 0 ),+ this.getMaxValue( unit, index )+ );- if ( this.getMaxValue( unit, index ) < parseInt( event.target.value, 10 ) ) {- return;- }-- rangeInput.value = valueInput.value;+ valueInput.value = clampedValue.toString();+ rangeInput.value = clampedValue.toString();
this.updateFill( rangeInput, index );
- this.options[ index ].fullValue = this.updateValue( element, valueInput.value + unit );+ this.options[ index ].fullValue = this.updateValue( element, `${ clampedValue }${ unit }` );
this.triggerValueChange( index );
} );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/src/settings-components/components/slider-component.js` around lines 81 -
93, The change handler currently only enforces the upper bound, allowing
negative or non-numeric values to reach updateValue; parse and normalize the
manual input first by reading event.target.value, converting with parseInt(...,
10), checking isNaN and clamping it between the minimum (use getMinValue if
available or 0) and this.getMaxValue(unit, index), then write the
normalized/clamped value back to valueInput.value and rangeInput.value before
calling this.updateFill(rangeInput, index), this.options[index].fullValue =
this.updateValue(element, valueInput.value + unit), and
this.triggerValueChange(index) so slider and hidden value stay in sync.
Verify each finding against the current code and only fix it if needed.
In
`@js/src/web-components/frm-range-slider-component/frm-range-slider-component.js`
around lines 319 - 330, The range input created in createSliderTrack currently
doesn't receive the passed ariaLabel, so assistive tech sees it as unnamed;
update createSliderTrack(value, maxValue) to accept or read the ariaLabel and
set it on the native element (e.g., slider.setAttribute('aria-label', ariaLabel)
or slider['ariaLabel']=ariaLabel) and do the same propagation in the other
range-creation function referenced around lines 437-442 (the alternate range
control factory) so both native input elements receive the aria-label from the
component prop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The current slider setting doesn't have good accessibility. Original comment: https://github.com/Strategy11/formidable-pro/pull/6388#issuecomment-4343715322
This also fixes the issue with saved value, reported in https://github.com/Strategy11/formidable-pro/pull/6388#issuecomment-4339438875
Summary by CodeRabbit
Refactor
Style